Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

IMPORTANT - Fixing collecting data from observers when using batch observer in first run #1470

Merged
merged 3 commits into from
Sep 3, 2020
Merged

IMPORTANT - Fixing collecting data from observers when using batch observer in first run #1470

merged 3 commits into from
Sep 3, 2020

Conversation

obecny
Copy link
Member

@obecny obecny commented Aug 27, 2020 •
edited
Loading

I discovered a bug when using batch observer. The batch observers needs to be run first so that the batcher can update observers first. After this the rest of observers and metrics should be run to collect the data. Otherwise when using the batch observers the meter.getBatcher().checkPointSet() was returning 0 records at first run.

@obecny obecny added the bug Something isn't working label Aug 27, 2020
@obecny obecny self-assigned this Aug 27, 2020
@obecny obecny added the size/M Denotes a PR that changes 30-99 lines, ignoring generated files. label Aug 27, 2020
@obecny obecny changed the title Fixing collection from observers when using batch observer Fixing collecting data from observers when using batch observer in first run Aug 27, 2020
@codecov
Copy link

codecov bot commented Aug 27, 2020 •
edited
Loading

Codecov Report

Merging #1470 into master will decrease coverage by 0.13%.
The diff coverage is 100.00%.

@@            Coverage Diff             @@
##           master    #1470      +/-   ##
==========================================
- Coverage   94.07%   93.93%   -0.14%     
==========================================
  Files         153      148       -5     
  Lines        4656     4401     -255     
  Branches      959      899      -60     
==========================================
- Hits         4380     4134     -246     
+ Misses        276      267       -9
Impacted Files Coverage Δ
...s/opentelemetry-metrics/src/BatchObserverMetric.ts 93.10% <ø> (ø)
packages/opentelemetry-metrics/src/Meter.ts 96.42% <100.00%> (+0.42%) ⬆️
packages/opentelemetry-metrics/src/Metric.ts 97.22% <100.00%> (+0.16%) ⬆️
packages/opentelemetry-metrics/src/export/types.ts 100.00% <100.00%> (ø)
...ages/opentelemetry-exporter-collector/src/types.ts
...lemetry-exporter-collector/src/transformMetrics.ts
.../opentelemetry-exporter-collector/src/transform.ts
...kages/opentelemetry-exporter-collector/src/util.ts
...ry-exporter-collector/src/CollectorExporterBase.ts
packages/opentelemetry-tracing/src/Span.ts 100.00% <0.00%> (+1.01%) ⬆️

@obecny obecny changed the title Fixing collecting data from observers when using batch observer in first run IMPORTANT - Fixing collecting data from observers when using batch observer in first run Aug 27, 2020
@obecny
Copy link
Member Author

obecny commented Aug 31, 2020

@open-telemetry/javascript-approvers this is important bug fix, please have a look, thx

@obecny obecny mentioned this pull request Aug 31, 2020
Closed
legendecas
legendecas approved these changes Sep 2, 2020
View reviewed changes
Copy link
Member

@legendecas legendecas left a comment •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

PR-wise LGTM.

However, I'm wondering if a batch observer should be an instance of Metric. According to https://github.com/open-telemetry/opentelemetry-specification/blob/master/specification/metrics/api.md#batch-observer the batch observers merely acts as an co-ordinator for other instruments. There may not be possibly instruments can be bound by the batch observer. Yet the implementation of BatchObserverMetric does inherit from Metric which can be bound to create new instruments -> those created instruments can not be updated by the callback of BatchObserverMetric.

In my thoughts, BatchObserverMetric can be simplified as separate categories with metrics and reduce confusion that BatchObserverMetric itself can be updated with values, and rename BatchObserverMetric to BatchObserver to indicate that itself is not a Metric.

@dyladan
Copy link
Member

dyladan commented Sep 2, 2020

I agree with @legendecas but let's get this bugfix merged then we can follow up if all agree

@obecny obecny mentioned this pull request Sep 3, 2020
Closed
@obecny
Copy link
Member Author

obecny commented Sep 3, 2020

PR-wise LGTM.

However, I'm wondering if a batch observer should be an instance of Metric. According to https://github.com/open-telemetry/opentelemetry-specification/blob/master/specification/metrics/api.md#batch-observer the batch observers merely acts as an co-ordinator for other instruments. There may not be possibly instruments can be bound by the batch observer. Yet the implementation of BatchObserverMetric does inherit from Metric which can be bound to create new instruments -> those created instruments can not be updated by the callback of BatchObserverMetric.

In my thoughts, BatchObserverMetric can be simplified as separate categories with metrics and reduce confusion that BatchObserverMetric itself can be updated with values, and rename BatchObserverMetric to BatchObserver to indicate that itself is not a Metric.

Lets investigate this in separate PR -> #1489

@obecny obecny merged commit 4c83b27 into open-telemetry:master Sep 3, 2020
@obecny obecny deleted the batch_observer_fix branch September 3, 2020 03:47
pichlermarc pushed a commit to dynatrace-oss-contrib/opentelemetry-js that referenced this pull request Dec 15, 2023
@haddasbronfman
chore(mysql2): support version 3 (open-telemetry#1470)
69d1f17
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@legendecas legendecas legendecas approved these changes

@vmarchaud vmarchaud vmarchaud approved these changes

@mwear mwear mwear approved these changes

@dyladan dyladan dyladan approved these changes

@markwolff markwolff Awaiting requested review from markwolff

@mayurkale22 mayurkale22 Awaiting requested review from mayurkale22

@naseemkullah naseemkullah Awaiting requested review from naseemkullah

@OlivierAlbertini OlivierAlbertini Awaiting requested review from OlivierAlbertini

Assignees

@obecny obecny

Labels
bug Something isn't working size/M Denotes a PR that changes 30-99 lines, ignoring generated files.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@obecny @dyladan @mwear @vmarchaud @legendecas